Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Display warning if clone job fails due to missing data view #117993

Merged

Conversation

peteharverson
Copy link
Contributor

Summary

Fixes regression introduced by #116455, adding back the warning toast if cloning an anomaly detection job fails because there is no data view matching the indices used in the datafeed.

Also adds a new functional test which would have caught the original regression.

image

Checklist

Fixes #117834

@peteharverson peteharverson added review :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v8.1.0 labels Nov 9, 2021
@peteharverson peteharverson self-assigned this Nov 9, 2021
@peteharverson peteharverson requested a review from a team as a code owner November 9, 2021 10:56
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

values: { jobId, dataViewTitle },
}
);
getToastNotifications().addDanger({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use getToastNotificationService instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched this one to getToastNotificationService in b67a413.

public async assertNoDataViewForCloneJobWarningToastExist() {
await retry.tryForTime(5000, async () => {
const toast = await testSubjects.find('mlCloneJobNoDataViewExistsWarningToast');
expect(toast).not.to.be(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to check the content of the toast?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok just to check the data-test-subj for now, and this is in line with other tests we have for toast notifications.

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think we can replace getToastNotifications with getToastNotificationService in a separate PR for the entire file

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and the fix worked fine.
Just two comments / suggestions for the test.

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ⚡

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@peteharverson
Copy link
Contributor Author

@elasticmachine merge upstream

@peteharverson peteharverson added the auto-backport Deprecated - use backport:version if exact versions are needed label Nov 11, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.5MB 3.5MB +345.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @peteharverson

@peteharverson peteharverson merged commit f440b3b into elastic:main Nov 11, 2021
@peteharverson peteharverson deleted the ml-clone-job-no-data-view-toast branch November 11, 2021 15:39
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 11, 2021
…stic#117993)

* [ML] Display warning if clone job fails due to missing data view

* [ML] Edits following review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 11, 2021
…7993) (#118373)

* [ML] Display warning if clone job fails due to missing data view

* [ML] Edits following review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Pete Harverson <pete@elastic.co>
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
…7993)

* [ML] Display warning if clone job fails due to missing data view

* [ML] Edits following review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Anomaly Detection ML anomaly detection :ml release_note:skip Skip the PR/issue when compiling release notes review v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ML] No warning shown if attempting to clone a job for which there is no data view
6 participants